-
Notifications
You must be signed in to change notification settings - Fork 149
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normative: Merge PrepareTemporalFields #2472
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2472 +/- ##
==========================================
+ Coverage 95.47% 95.50% +0.02%
==========================================
Files 20 20
Lines 10929 10931 +2
Branches 2034 2035 +1
==========================================
+ Hits 10435 10440 +5
+ Misses 432 430 -2
+ Partials 62 61 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this means that even in an engine without 402, it would be an error to provide only one of era and eraYear, right? (ie, only neither or both is allowed)
spec/abstractops.html
Outdated
1. If _requiredFields_ is not ~partial~, then | ||
1. Let _era_ be ? Get(_result_, *"era"*). | ||
1. Let _eraYear_ be ? Get(_result_, *"eraYear"*). | ||
1. If _era_ is *undefined* and _eraYear_ is not *undefined*, throw a *RangeError* exception. | ||
1. If _era_ is not *undefined* and _eraYear_ is *undefined*, throw a *RangeError* exception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per #2465 (comment) we'll remove this part because it is already done in dateFromFields/yearMonthFromFields/monthDayFromFields anyway, after each call to PrepareTemporalFields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a harm in validating it here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it "leaks" calendar-specific logic outside of Temporal.Calendar methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there likely to be any calendar that supports one of these two properties but not both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Calendars either use eras or they won't. If they do use eras (which is, BTW, the vast majority of CLDR calendars) then era
/eraYear
are used as a pair. One of then without the other is not meaningful.
This means that discussions relating to validation and eras are focused on relatively narrow questions:
- In non-era-using calendars, or in 262 without 402, what should happen if callers provide (in a property bag passed to
from
,with
, etc.) one ofera
/eraYear
without the other? Throw? Ignore? - Where exactly should the validation take place that ensures that those two properties are either both present or both
undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that unknown properties should be ignored, but known ones should be validated as early as possible.
Given that these two properties are really just one property, that contains two items, and should only come as a pair, it seems reasonable to me to validate for them here as well, since they're known property names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is validated already, this just removes a duplicate validation. I get what you are saying about validating as early as possible, but I don't think that weighs against having calendar-specific logic live outside of Temporal.Calendar methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This AO is called "PrepareTemporalFields" and Calendar is part of Temporal - since this is logic that would apply to any era-using calendar, it doesn't feel calendar-specific to me, it feels calendar-general, which doesn't imo need to be contained to calendar methods.
This achieved consensus at the 2021-01-31 TC39 plenary meeting. @ljharb, I assume your open question was resolved by the question you asked during the presentation about I think before merging this we should have a test262 test to verify it. Something based on the code sample I gave in my slides (https://ptomato.name/talks/tc39-2023-01/#12) would be sufficient, I think; that code sample should work for the ISO calendar regardless of whether the implementation has other calendars or just ISO. |
@ptomato yes, as long as ecma262 has zero mention of era/eraYear, and all of it is in ecma402, then I can tolerate it :-) (altho my preference would be for everything to be in 262, and for 402 to only patch methods or AOs as needed to add intl-specific logic). |
b9e6313
to
ee0cf92
Compare
Avoiding two different version of PrepareTemporalFields for unneeded reason by adding the era and eraYear check into the ECAM262 version and remove the ECMA402 version.
ee0cf92
to
4dff8ed
Compare
Tests are in: tc39/test262#3792 |
4dff8ed
to
e4b2001
Compare
e4b2001
to
d8c8bb1
Compare
Thanks, merging now! |
Avoiding two different versions of PrepareTemporalFields for unneeded reason by adding the 'era' and 'eraYear' check into the ECAM262 version and remove the ECMA402 version.
Close #2465